Fix issue where Domain to UI model converter double reports the same …#579
Fix issue where Domain to UI model converter double reports the same …#579yurishkuro merged 4 commits intomasterfrom
Conversation
…Span Reference Signed-off-by: Won Jun Jang <wjang@uber.com>
black-adder
requested review from
pavolloffay,
vprithvi and
yurishkuro
as code owners
ghost
assigned
model/converter/json/from_domain.go
Outdated
| } | ||
| for _, ref := range span.References { | ||
| out = append(out, json.Reference{ | ||
| // TODO if the parentRef was wrongly added as a ChildOf ref above, this could |
There was a problem hiding this comment.
For this TODO, it might be easier to keep track of parentRef but not add it until we're done with this for loop and if the reference isn't added, add it post. This way, if the reference should be a FollowsFrom, we don't double report.
There was a problem hiding this comment.
do you want to fix it for good then?
iirc the only reason ParentSpanID even exists in the model is an optimization. In our deployment we hardly have any apps that create FollowsFrom references, so the overwhelming majority of spans have just a single parent of the child-of type. Instead of representing that link as a full blown Reference object we simply record the ParentSpanID (a single uint64). |
model/converter/json/from_domain.go
Outdated
| SpanID: json.SpanID(ref.SpanID.String()), | ||
| } | ||
| out = append(out, newRef) | ||
| if newRef.TraceID == json.TraceID(span.TraceID.String()) && |
There was a problem hiding this comment.
comparison via strings is inefficient - you have the strongly typed ref in scope, why not use that?
model/converter/json/from_domain.go
Outdated
| } | ||
| } | ||
| if span.ParentSpanID != 0 && !preserveParentID && !parentRefAdded { | ||
| // TODO this (wrongly) assumes that the reference type is always ChildOf |
There was a problem hiding this comment.
it assumes that correctly, because if ParentSpanID != 0 but there are no other references then that ParentSpanID does refer to child-of type (you may want to leave that as a comment)
model/converter/json/from_domain.go
Outdated
| SpanID: json.SpanID(ref.SpanID.String()), | ||
| }) | ||
| if ref.TraceID == span.TraceID && ref.SpanID == span.ParentSpanID { | ||
| // Check if the parent reference already exists |
There was a problem hiding this comment.
not sure this comment explains anything, maybe remove it?
| parentRefAdded = true | ||
| } | ||
| } | ||
| if span.ParentSpanID != 0 && !preserveParentID && !parentRefAdded { |
There was a problem hiding this comment.
not directly related to this diff, but I see that the only time preserveParentID == true happens is when this conversion is called from ES storage WriteSpan function. What was the reason why we needed to preserve ParentSpanID in ES? Same performance/storage optimization?
There was a problem hiding this comment.
don't recall the exact reason but it probably has to do with keeping the es schema the same as the zipkin one.
|
Incidentally, I wonder if it would've been better to write an Adjuster for the domain model instead, and not have any of this business logic in the toJSON conversion. |
Signed-off-by: Won Jun Jang <wjang@uber.com>
black-adder
force-pushed
the
fix_double_ref_issue
branch
from
fcb3905 to
8926820
Compare
|
btw was there a ticket describing what the problem this was causing? |
|
only internally, I thought it was an internal build issue. I'll put one up on github |
3 participants
…Span Reference
Problem was that the converter removes the parentSpanID and replaces it with a parent child-of Reference. But if the child-of reference is already present in the model span, it gets double reported.
It seems like the whole
if span.ParentSpanID != 0 && !preserveParentID {code path could be removed. Can any part of the jaeger system send in a span where ParentSpanID is filled out but the child-of reference is not present? Also, the logic here makes a couple of faulty assumptions.Signed-off-by: Won Jun Jang wjang@uber.com